Skip to content

Conversation

@hoophalab
Copy link
Contributor

@hoophalab hoophalab commented Jan 5, 2026

Description

This PR implements the reverse proxy for log ingestor. For example, the user can access log ingestor through http://localhost:3001/log_ingestor/s3_scanner

This PR doesn't have to be included in the release.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  1. Set log_input.type as s3 in clp-config
  2. http://localhost:3001/log_ingestor/ returns log-ingestor is running
  3. Successfully ingested logs through http://localhost:3001/log_ingestor/s3_scanner

Summary by CodeRabbit

  • New Features

    • Config-driven log ingestor routing: an automatic reverse-proxy route is enabled when S3 logging is selected; proxy setup is skipped for other log inputs.
  • Security

    • Enhanced TLS support by integrating the AWS-LC cryptography provider for stronger and more compatible TLS.
  • Chores

    • Deployment: log ingestor hostname made configurable via environment variable for easier operations.

✏️ Tip: You can customize this high-level summary in your review settings.


@hoophalab hoophalab requested a review from a team as a code owner January 5, 2026 19:46
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Walkthrough

Router builder now accepts a config reference; when config.logs_input indicates S3 it installs the AWS‑LC crypto provider and mounts an axum reverse proxy at /log_ingestor. Two new dependencies (axum-reverse-proxy and rustls with aws_lc_rs) were added to the API server crate.

Changes

Cohort / File(s) Summary
Dependency additions
components/api-server/Cargo.toml
Added axum-reverse-proxy = "1.1.1" and rustls = { version = "0.23.36", features = ["aws_lc_rs"] }.
Binary startup
components/api-server/src/bin/api_server.rs
Call updated: api_server::routes::from_client(client)api_server::routes::from_client(client, &config).
Route configuration & proxy
components/api-server/src/routes.rs
from_client now accepts &Config; when config.logs_input == S3 it installs the AWS‑LC crypto provider (documented panic on failure), resolves a hostname, and creates/merges an axum-reverse-proxy route for /log_ingestor pointing to http://{hostname}:{port}.
Deployment env
tools/deployment/package/docker-compose-all.yaml
Added environment variable CLP_LOG_INGESTOR_HOSTNAME: "log_ingestor" to the api_server service.

Sequence Diagram(s)

sequenceDiagram
    participant App as Application
    participant Routes as Routes module
    participant Config as Config
    participant Crypto as AWS‑LC provider
    participant Proxy as ReverseProxy
    participant Router as Axum Router

    App->>Routes: from_client(client, &config)
    Routes->>Config: read logs_input
    alt logs_input == S3
        rect rgba(200,230,255,0.5)
        Note right of Routes: initialize crypto provider (may panic on failure)
        Routes->>Crypto: install provider
        Crypto-->>Routes: installed
        Routes->>Proxy: create ReverseProxy for /log_ingestor -> log_ingestor:{port}
        Proxy-->>Routes: proxy route ready
        end
        Routes->>Router: merge proxy routes
    else logs_input != S3
        Note right of Routes: skip crypto & proxy setup
    end
    Router-->>App: return configured Router
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change in the PR: adding a reverse proxy for the log ingestor to the API server. It is concise, specific, and clearly conveys the primary objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Fix all issues with AI Agents 🤖
In @components/api-server/src/routes.rs:
- Around line 60-64: The ReverseProxy instantiation hardcodes the hostname
"log_ingestor"; update it to read host from the configuration instead: add a
host field to the log_ingestor config (e.g., config.log_ingestor.host) in
clp_rust_utils, then use that host together with config.log_ingestor.port when
building the URL passed to axum_reverse_proxy::ReverseProxy::new (replace
"log_ingestor" with the configurable host using format! to construct
"http://{host}:{port}"). Ensure the config type and any deserialization/defaults
are updated so host is available where ReverseProxy::new is called.
- Around line 33-36: Update the doc comment in
components/api-server/src/routes.rs where the Panics section reads "failes" and
correct the typo to "fails" so the sentence reads "Panics if the log ingestor is
enabled but fails to install `aws_lc_rs` as the crypto provider."; ensure you
only change the misspelled word in the existing doc comment (the Panics block)
and leave the rest unchanged.
- Around line 57-59: The call to
rustls::crypto::aws_lc_rs::default_provider().install_default() currently uses
.expect(...), which will panic if a provider is already installed; change it to
handle the Err variant instead (e.g., check if let Err(_) =
rustls::crypto::aws_lc_rs::default_provider().install_default() and log a debug
or warn message like "Crypto provider already installed by another component")
so the API server doesn't crash on duplicate provider installation.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82af8f5 and f55bb73.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • components/api-server/Cargo.toml
  • components/api-server/src/bin/api_server.rs
  • components/api-server/src/routes.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.

Applied to files:

  • components/api-server/src/routes.rs
  • components/api-server/Cargo.toml
  • components/api-server/src/bin/api_server.rs
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Applied to files:

  • components/api-server/Cargo.toml
🧬 Code graph analysis (1)
components/api-server/src/bin/api_server.rs (1)
components/api-server/src/routes.rs (1)
  • from_client (37-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: package-image
  • GitHub Check: rust-checks
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: check-generated
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (6)
components/api-server/src/bin/api_server.rs (1)

99-99: LGTM!

The updated function call correctly passes the configuration reference to match the new signature in routes.rs.

components/api-server/src/routes.rs (4)

12-12: LGTM!

The added imports are necessary for the configuration-driven routing logic introduced in this PR.


37-37: LGTM!

The function signature correctly adds the configuration parameter needed for conditional proxy setup. The caller has been updated accordingly in api_server.rs.


53-55: LGTM!

The pattern matching correctly ensures the reverse proxy is only set up when S3 logging is configured. The early return pattern is idiomatic and efficient.


60-60: No validation concern required; log_ingestor is a required field in Config.

The log_ingestor field is not optional—it is a required field of type LogIngestor in the Config struct, and port is a required u16 field within LogIngestor. Both are guaranteed to exist at runtime and cannot cause a panic when accessed. No additional validation is needed.

components/api-server/Cargo.toml (1)

44-45: The pinned dependency versions are current and secure.

Both axum-reverse-proxy (1.1.1) and rustls (0.23.36) are at their latest stable versions with no known security vulnerabilities.

Comment on lines +57 to +59
rustls::crypto::aws_lc_rs::default_provider()
.install_default()
.expect("Failed to install default crypto provider");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

rustls 0.23.36 crypto provider install_default API documentation error handling

💡 Result:

Short answer: install_default returns Result<(), Arc>; an Ok(()) means this call successfully installed the provider for the process, and Err(arc) is returned when installation fails (e.g. a default provider was already installed). The method may be called successfully at most once per process, so callers should handle the Err by either ignoring it (let _ = …), using the returned Arc to inspect the already-installed provider, or reporting the conflict (unwrap/expect will panic). [1][2]

Sources:

  • rustls CryptoProvider docs (install_default signature + usage notes). [1]
  • docs.rs rustls crate (impl docs and source showing install_default returns Result<(), Arc> and "at most once" note). [2]

🏁 Script executed:

fd "routes.rs" --type f

Repository: y-scope/clp

Length of output: 128


🏁 Script executed:

cat -n components/api-server/src/routes.rs | head -80

Repository: y-scope/clp

Length of output: 3090


🏁 Script executed:

rg "install_default\|crypto::aws_lc_rs" -A 3 -B 3 components/api-server/src/

Repository: y-scope/clp

Length of output: 37


🏁 Script executed:

rg "crypto::" --type rs -l

Repository: y-scope/clp

Length of output: 81


🏁 Script executed:

rg "install_default" --type rs -B 5 -A 5

Repository: y-scope/clp

Length of output: 81


🏁 Script executed:

rg "install_default" -B 5 -A 5

Repository: y-scope/clp

Length of output: 851


🏁 Script executed:

rg "crypto::" -B 3 -A 3

Repository: y-scope/clp

Length of output: 522


🏁 Script executed:

rg "from_client\|install_default" -B 2 -A 8

Repository: y-scope/clp

Length of output: 37


🏁 Script executed:

find . -name "Cargo.toml" -exec grep -l "rustls" {} \;

Repository: y-scope/clp

Length of output: 89


🏁 Script executed:

cat components/api-server/Cargo.toml | grep -A 5 -B 5 "rustls"

Repository: y-scope/clp

Length of output: 343


🏁 Script executed:

rg "from_client" -B 2 -A 15

Repository: y-scope/clp

Length of output: 2113


Handle the error case when a crypto provider is already installed.

The install_default() method returns Result<(), Arc<CryptoProvider>> and returns an Err when a default provider is already installed. Using .expect() will panic on this error, crashing the API server at startup if another component has already installed a crypto provider.

Gracefully handle the case where a provider is already installed:

Suggested fix
if let Err(_) = rustls::crypto::aws_lc_rs::default_provider().install_default() {
    tracing::debug!("Crypto provider already installed by another component");
}

Alternatively, you can log a warning if preferred, or simply ignore the error with let _ = ... if a duplicate installation is expected in your deployment model.

🤖 Prompt for AI Agents
In @components/api-server/src/routes.rs around lines 57-59, The call to
rustls::crypto::aws_lc_rs::default_provider().install_default() currently uses
.expect(...), which will panic if a provider is already installed; change it to
handle the Err variant instead (e.g., check if let Err(_) =
rustls::crypto::aws_lc_rs::default_provider().install_default() and log a debug
or warn message like "Crypto provider already installed by another component")
so the API server doesn't crash on duplicate provider installation.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI Agents
In @components/api-server/src/routes.rs:
- Around line 52-55: Add a concise comment above the early return that checks
LogsInput::S3 explaining why the reverse proxy is only required for S3 log
input: note that when config.logs_input is S3 the service must proxy requests to
the external S3-backed log ingestor (or internal ingestor accessing S3 objects)
and therefore the reverse proxy/router needs to be set up, whereas other
LogsInput variants do not require that HTTP proxying; reference the
LogsInput::S3 check and the router variable so future readers understand the
S3-specific dependency on the log ingestor and why we short-circuit for other
inputs.
- Around line 57-64: The code adds the aws_lc_rs crypto provider because
axum_reverse_proxy::ReverseProxy enables TLS by default even when the target URL
uses http://; either keep the existing
rustls::crypto::aws_lc_rs::default_provider().install_default() call or remove
that provider and disable TLS in the axum-reverse-proxy dependency to avoid
pulling the TLS lib unnecessarily; to change, update the Cargo.toml entry for
axum-reverse-proxy to explicitly set features = [] (e.g., axum-reverse-proxy = {
version = "1.1.1", features = [] }) or retain the install_default() call near
where ReverseProxy::new("/log_ingestor", &format!("http://log_ingestor:{port}"))
is created so the required crypto provider is present.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2723b1 and 8e0a10f.

📒 Files selected for processing (1)
  • components/api-server/src/routes.rs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-29T21:00:07.757Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.

Applied to files:

  • components/api-server/src/routes.rs
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.

Applied to files:

  • components/api-server/src/routes.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: package-image
  • GitHub Check: check-generated
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: rust-checks
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
components/api-server/src/routes.rs (2)

12-12: LGTM!

The imports are necessary for the new configuration-driven proxy setup.


33-37: LGTM!

The documentation clearly describes the panic condition, and the function signature update appropriately accepts the configuration reference needed for conditional proxy setup.

Comment on lines +52 to +55

let LogsInput::S3 { .. } = &config.logs_input else {
return Ok(router);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Document why the reverse proxy is S3-specific.

The early return ensures the proxy is only set up for S3 logs input, but it's not immediately clear why this functionality is tied specifically to S3. Consider adding a comment explaining the relationship between S3 logs input and the need for a reverse proxy to the log ingestor.

🔎 Suggested addition
+// The reverse proxy for the log ingestor is only needed when using S3 as the logs input
+// source, as the log ingestor service handles S3 scanning and ingestion.
 let LogsInput::S3 { .. } = &config.logs_input else {
     return Ok(router);
 };
🤖 Prompt for AI Agents
In @components/api-server/src/routes.rs around lines 52-55, Add a concise
comment above the early return that checks LogsInput::S3 explaining why the
reverse proxy is only required for S3 log input: note that when
config.logs_input is S3 the service must proxy requests to the external
S3-backed log ingestor (or internal ingestor accessing S3 objects) and therefore
the reverse proxy/router needs to be set up, whereas other LogsInput variants do
not require that HTTP proxying; reference the LogsInput::S3 check and the router
variable so future readers understand the S3-specific dependency on the log
ingestor and why we short-circuit for other inputs.

Comment on lines 57 to 64
rustls::crypto::aws_lc_rs::default_provider()
.install_default()
.expect("Failed to install default crypto provider");
let port = &config.log_ingestor.port;
let proxy = axum_reverse_proxy::ReverseProxy::new(
"/log_ingestor",
&format!("http://log_ingestor:{port}"),
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

cd components/api-server && cat Cargo.toml | head -50

Repository: y-scope/clp

Length of output: 1366


🏁 Script executed:

cat -n components/api-server/src/routes.rs | head -80

Repository: y-scope/clp

Length of output: 3089


🏁 Script executed:

rg "axum.*reverse.*proxy|ReverseProxy" components/api-server/src/ -A 3 -B 3

Repository: y-scope/clp

Length of output: 572


🏁 Script executed:

rg "log_ingestor" components/api-server/src/ -B 2 -A 2

Repository: y-scope/clp

Length of output: 646


🏁 Script executed:

web_search
Search for axum-reverse-proxy crate documentation and TLS requirements

Repository: y-scope/clp

Length of output: 150


🏁 Script executed:

rg "https?://" components/api-server/src/ -B 2 -A 2

Repository: y-scope/clp

Length of output: 407


🏁 Script executed:

rg "log_ingestor" components/ -r --type rust -B 3 -A 3 | head -100

Repository: y-scope/clp

Length of output: 8273


🏁 Script executed:

find components -name "*.rs" -type f | xargs grep -l "ReverseProxy" | head -5

Repository: y-scope/clp

Length of output: 90


🌐 Web query:

axum-reverse-proxy crate documentation TLS rustls requirements

💡 Result:

Summary — axum-reverse-proxy TLS requirements

  • By default the crate enables TLS via rustls (via hyper-rustls). Use the default feature (tls) or explicitly enable features = ["tls"]. [1][2]
  • The crate exposes an optional native-tls feature to use the platform TLS (OpenSSL / Secure Transport / SChannel) instead of rustls; if both tls and native-tls are enabled, native-tls takes precedence. [1][2]
  • You can disable TLS entirely (features = [] ) to build HTTP-only. [1][2]

Sources: docs.rs axum-reverse-proxy documentation and the crate GitHub README. [1][2]

References
[1] docs.rs — axum-reverse-proxy TLS configuration.
[2] GitHub — tom-lubenow/axum-reverse-proxy README.


🏁 Script executed:

rg "aws_sdk_s3|s3::|S3" components/api-server/src/routes.rs -B 2 -A 2

Repository: y-scope/clp

Length of output: 198


🏁 Script executed:

cat -n components/api-server/src/lib.rs | head -40

Repository: y-scope/clp

Length of output: 118


🏁 Script executed:

rg "https" components/ --type rust -i | grep -i "log.ingestor\|ingestor"

Repository: y-scope/clp

Length of output: 37


🏁 Script executed:

grep -r "features" components/api-server/Cargo.toml | grep axum

Repository: y-scope/clp

Length of output: 163


The crypto provider is required because axum-reverse-proxy has TLS enabled by default.

The existing docstring (line 36) already documents why aws_lc_rs is needed: the axum-reverse-proxy crate enables TLS support by default via the tls feature. Although the proxy target uses http://, the TLS library is still required by the underlying dependency.

If the log ingestor endpoint is HTTP-only and TLS support is not needed elsewhere, consider disabling the TLS feature: change the dependency to axum-reverse-proxy = { version = "1.1.1", features = [] } to reduce unnecessary dependencies.

🤖 Prompt for AI Agents
In @components/api-server/src/routes.rs around lines 57-64, The code adds the
aws_lc_rs crypto provider because axum_reverse_proxy::ReverseProxy enables TLS
by default even when the target URL uses http://; either keep the existing
rustls::crypto::aws_lc_rs::default_provider().install_default() call or remove
that provider and disable TLS in the axum-reverse-proxy dependency to avoid
pulling the TLS lib unnecessarily; to change, update the Cargo.toml entry for
axum-reverse-proxy to explicitly set features = [] (e.g., axum-reverse-proxy = {
version = "1.1.1", features = [] }) or retain the install_default() call near
where ReverseProxy::new("/log_ingestor", &format!("http://log_ingestor:{port}"))
is created so the required crypto provider is present.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @components/api-server/src/routes.rs:
- Around line 63-70: The code reads LOG_INGESTOR_HOSTNAME_ENV_VAR into hostname
but doesn't guard against an empty string, causing a malformed URL; update the
logic around hostname to treat empty or whitespace-only values as missing: fetch
the env var via std::env::var(LOG_INGESTOR_HOSTNAME_ENV_VAR), .map(|s|
s.trim().to_owned()), then if the result is Err or the trimmed string is empty
log the existing info message and set hostname =
DEFAULT_LOG_INGESTOR_HOSTNAME.to_owned(); ensure references to
LOG_INGESTOR_HOSTNAME_ENV_VAR, DEFAULT_LOG_INGESTOR_HOSTNAME and the hostname
variable are used so downstream URL construction uses a valid non-empty
hostname.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e0a10f and 9004e06.

📒 Files selected for processing (2)
  • components/api-server/src/routes.rs
  • tools/deployment/package/docker-compose-all.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-19T05:03:42.629Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1818
File: tools/deployment/package-helm/templates/configmap.yaml:12-12
Timestamp: 2025-12-19T05:03:42.629Z
Learning: In the y-scope/clp Helm chart, the `api_server.host` configuration field in the ConfigMap (tools/deployment/package-helm/templates/configmap.yaml) is unused because no other k8s internal services need to reach the API server—it's only accessed from outside the cluster via NodePort.

Applied to files:

  • tools/deployment/package/docker-compose-all.yaml
📚 Learning: 2025-11-03T16:17:40.223Z
Learnt from: hoophalab
Repo: y-scope/clp PR: 1535
File: components/clp-rust-utils/src/clp_config/package/config.rs:47-61
Timestamp: 2025-11-03T16:17:40.223Z
Learning: In the y-scope/clp repository, the `ApiServer` struct in `components/clp-rust-utils/src/clp_config/package/config.rs` is a Rust-native configuration type and does not mirror any Python code, unlike other structs in the same file (Config, Database, ResultsCache, Package) which are mirrors of Python definitions.

Applied to files:

  • tools/deployment/package/docker-compose-all.yaml
  • components/api-server/src/routes.rs
📚 Learning: 2025-07-29T21:00:07.757Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.

Applied to files:

  • components/api-server/src/routes.rs
📚 Learning: 2025-10-17T19:59:35.299Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1178
File: components/clp-package-utils/clp_package_utils/controller.py:757-757
Timestamp: 2025-10-17T19:59:35.299Z
Learning: In the y-scope/clp repository, prefer long-form command-line flags (e.g., `--recursive`) over short-form flags (e.g., `-R`) for readability, even if the long form is GNU-specific.

Applied to files:

  • components/api-server/src/routes.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: package-image
  • GitHub Check: rust-checks
  • GitHub Check: check-generated
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (2)
tools/deployment/package/docker-compose-all.yaml (1)

576-576: LGTM! Environment variable correctly references the log ingestor service.

The CLP_LOG_INGESTOR_HOSTNAME environment variable is set to "log_ingestor", which matches the hostname of the log-ingestor service defined at line 609. This enables proper service discovery for the reverse proxy in the API server.

components/api-server/src/routes.rs (1)

71-76: LGTM! Reverse proxy correctly configured.

The reverse proxy is properly instantiated with the /log_ingestor prefix and dynamically constructs the backend URL using the configured hostname and port. This implementation aligns with the PR objectives to expose the log ingestor under the API server.

Comment on lines +63 to +70
let hostname = std::env::var(LOG_INGESTOR_HOSTNAME_ENV_VAR);
if hostname.is_err() {
tracing::info!(
"`{LOG_INGESTOR_HOSTNAME_ENV_VAR}` is not configured. Using \
`{DEFAULT_LOG_INGESTOR_HOSTNAME}` instead."
);
}
let hostname = hostname.unwrap_or_else(|_| DEFAULT_LOG_INGESTOR_HOSTNAME.to_owned());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Validate that the hostname is not empty.

If the environment variable CLP_LOG_INGESTOR_HOSTNAME is set to an empty string, the current code will use that empty value, resulting in a malformed URL http://:3002 at line 74. Consider validating that the hostname is non-empty before using it.

🔍 Proposed fix
 let hostname = std::env::var(LOG_INGESTOR_HOSTNAME_ENV_VAR);
 if hostname.is_err() {
     tracing::info!(
         "`{LOG_INGESTOR_HOSTNAME_ENV_VAR}` is not configured. Using \
          `{DEFAULT_LOG_INGESTOR_HOSTNAME}` instead."
     );
 }
-let hostname = hostname.unwrap_or_else(|_| DEFAULT_LOG_INGESTOR_HOSTNAME.to_owned());
+let hostname = hostname
+    .ok()
+    .filter(|h| !h.is_empty())
+    .unwrap_or_else(|| {
+        tracing::info!(
+            "`{LOG_INGESTOR_HOSTNAME_ENV_VAR}` is empty or not configured. Using \
+             `{DEFAULT_LOG_INGESTOR_HOSTNAME}` instead."
+        );
+        DEFAULT_LOG_INGESTOR_HOSTNAME.to_owned()
+    });
🤖 Prompt for AI Agents
In @components/api-server/src/routes.rs around lines 63 - 70, The code reads
LOG_INGESTOR_HOSTNAME_ENV_VAR into hostname but doesn't guard against an empty
string, causing a malformed URL; update the logic around hostname to treat empty
or whitespace-only values as missing: fetch the env var via
std::env::var(LOG_INGESTOR_HOSTNAME_ENV_VAR), .map(|s| s.trim().to_owned()),
then if the result is Err or the trimmed string is empty log the existing info
message and set hostname = DEFAULT_LOG_INGESTOR_HOSTNAME.to_owned(); ensure
references to LOG_INGESTOR_HOSTNAME_ENV_VAR, DEFAULT_LOG_INGESTOR_HOSTNAME and
the hostname variable are used so downstream URL construction uses a valid
non-empty hostname.

@junhaoliao junhaoliao modified the milestones: Backlog, February 2026 Jan 19, 2026
Copy link
Member

@junhaoliao junhaoliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick question: with this implemented, is it possible to remove the host / port mapping for the log ingestor in docker compose / k8s?

if so, we don't have to update this PR. i can help with the implementation as well

@hoophalab
Copy link
Contributor Author

quick question: with this implemented, is it possible to remove the host / port mapping for the log ingestor in docker compose / k8s?

if so, we don't have to update this PR. i can help with the implementation as well

Probably cannot remove docker compose mapping. Otherwise the api_server cannot connect to log ingestor, which is in another container.

k8s: We should already be using pod-to-pod communication provided by CNI (correct me if I'm wrong). So NodePort of the log ingestor service can be removed.

@junhaoliao
Copy link
Member

Probably cannot remove docker compose mapping. Otherwise the api_server cannot connect to log ingestor, which is in another container.

i can be wrong but cross container communication should be possible because all the containers do use a shared network started by the same docker compose project. when we remove the container to host port mapping, communications among containers shouldn't be impacted. we can look more into those when we implement

either way, let's create an issue to track, if we all agree the log ingestor should no longer be directly accessed by users

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tools/deployment/package/docker-compose-all.yaml (1)

569-608: Add depends_on: log-ingestor when S3 ingestion is enabled.

The api-server service configures a reverse proxy to log-ingestor only when S3 input is enabled (line 56 in routes.rs), but currently has no depends_on relationship ensuring log-ingestor is healthy before starting. This creates a race condition where api-server may start before log-ingestor is ready, causing /log_ingestor/* proxy requests to fail initially.

While axum_reverse_proxy likely returns 5xx responses on upstream connection failures rather than crashing the server, explicitly ordering the startup improves reliability. Consider using a conditional depends_on strategy or ensuring the Docker Compose environment reflects your deployment's S3 configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants